Add an 'extras' property to comments #807
Conversation
There was a problem hiding this comment.
@crazytonyli I think this is a good start, but it looks like we have quite a few failing tests.
- We could consider marking the field with
#[WpContextualOption]because there won't always be extra fields. I wonder if that'd also help with de-serializing speed. - We might want to introduce a new type to hold the extras, instead of directly using
JsonValue. This might be changed to be auniffi::Objectwith some helpers in the future and having a type in advance could make things easier for us. It'd also look a bit more consistent when we use it in other types. Note that you probably need to use#[serde(transparent)]for this to work, but I don't know if#[serde(transparent)]&#[serde(flatten)]works together or not. - The field ordering might matter, as I believe it does for de-serializing
enums. That'd be a bit of an issue, because if we accidentally put this field above others, the ones below might always be set toNone. If this is an issue, we could add a check inWpContextproc macro to ensure that if there is anextrafield, it's at the end. Having a specific type would also help with this. - If we have a separate type, we could consider moving the
metafield to it. I am not so sure about this though, so just dropping it as an idea for now.
It does not seem so: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=b62029454b6f50395fbb3dfd17a8d4b4. BTW, I'll leave this PR open until we have an API endpoint that requires this change, like the WP.com /wp/v2/comments (see https://linear.app/a8c/issue/AINFRA-939). |
crazytonyli
force-pushed
the
parse-extra-fields
branch
from
450e9d2 to
56f6562
Compare
|
@oguzkocer This PR is ready to be reviewed and merged. The |
| @@ -528,6 +528,9 @@ pub struct SparseComment { | |||
| pub comment_type: Option<CommentType>, | |||
| #[WpContext(edit, embed, view)] | |||
| pub author_avatar_urls: Option<HashMap<UserAvatarSize, WpResponseString>>, | |||
| #[serde(flatten)] | |||
| #[WpContext(edit, embed, view)] | |||
| pub additional_fields: Option<Arc<AnyJson>>, | |||
There was a problem hiding this comment.
This field is included in the generated SparseCommentFields, which it should not be. I guess we'll have to implement a logic to exclude certain fields. We can either check the #[serde(flatten)] attribute (if that's possible), or add a new attribute like #[WpContextualExcludeFromFields]. What do you think? @oguzkocer
There was a problem hiding this comment.
I was checking what needed to be done for this and figured I'd quickly implement it, so you don't have to do the same: #833
There was a problem hiding this comment.
Thanks!
I have got this error using the new attribute, do you mind having a look?
$ cargo build
Compiling wp_api v0.1.0 (/Users/tonyli/Projects/wordpress-rs/wp_api)
error[E0599]: no variant or associated item named `AdditionalFields` found for enum `SparseCommentFieldWithEditContext` in the current scope
--> wp_api/src/comments.rs:495:57
|
495 | #[derive(Debug, Serialize, Deserialize, uniffi::Record, WpContextual)]
| ^^^^^^^^^^^^
| |
| variant or associated item not found in `SparseCommentFieldWithEditContext`
| variant or associated item `AdditionalFields` not found for this enum
|
= note: this error originates in the derive macro `WpContextual` (in Nightly builds, run with -Z macro-backtrace for more info)
There was a problem hiding this comment.
Fixed in c0cbafd. (after checking with @crazytonyli that it'd be OK for me to push directly to his branch)
There was a problem hiding this comment.
@crazytonyli This is mostly looking good. I've left some suggestions. Let me know what you think!
| @@ -528,6 +528,9 @@ pub struct SparseComment { | |||
| pub comment_type: Option<CommentType>, | |||
| #[WpContext(edit, embed, view)] | |||
| pub author_avatar_urls: Option<HashMap<UserAvatarSize, WpResponseString>>, | |||
| #[serde(flatten)] | |||
| #[WpContext(edit, embed, view)] | |||
| pub additional_fields: Option<Arc<AnyJson>>, | |||
There was a problem hiding this comment.
I was checking what needed to be done for this and figured I'd quickly implement it, so you don't have to do the same: #833
crazytonyli
force-pushed
the
parse-extra-fields
branch
from
56f6562 to
d685335
Compare
Exclude fields marked with WpContextualExcludeFromFields from integration test helper generation to prevent referencing non-existent enum variants.
|
Hey @oguzkocer , I have addressed your comments. Do you mind having another look? Thanks. |
@oguzkocer I tried the "serde flatten" solution you mentioned in our 1:1, and it worked out great. One minor downside is that there will be additional parsing work required for all API endpoints. I tried to use
RawValueto avoid that, but it does not seem to work withserde(flatten)(see serde-rs/json#1252).WP.com endpoints may return extra information in the responses. We'll need to parse that information to support the app's functionalities. This PR introduces a mechanism to support that. We can add this block of code to the models if needed: